-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[util-linux-libuuid] Change CMake target from LibUUID::LibUUID to libuuid::libuuid #18559
[util-linux-libuuid] Change CMake target from LibUUID::LibUUID to libuuid::libuuid #18559
Conversation
…uuid::libuuid * This is in line with what conan recipes seem to expect, which will minimise the number of patches required. LibUUID was chosen as the target name given this is what's used internally within cmake, but this isn't a public interface and so there is no common usage to take guidance from. Closes conan-io#18554
This comment has been minimized.
This comment has been minimized.
self.cpp_info.set_property("cmake_target_name", "libuuid::libuuid") | ||
self.cpp_info.set_property("cmake_file_name", "libuuid") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.cpp_info.set_property("cmake_target_name", "libuuid::libuuid") | |
self.cpp_info.set_property("cmake_file_name", "libuuid") |
Where does it come from? Why not just keep recipename::recipename
? It's conan-center convention when a library doesn't have any official config file or Find module file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This recipe provides the libuuid
interface and replaces/deprecates the libuuid
recipe. The failing libraries for this migration have already been patched for libuuid::libuuid
, so this is the smallest change that would get everything on CCI functional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not because it's the smallest change that it the good one. You can also use set_property() method of CMakeDeps in these recipes to override these names in order to satisfy these patches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example if in downstream library there is a custom Findlibuuid.cmake
and its CMakeLists.txt uses find_package(libuuid)
and libuuid::libuuid
target:
def generate(self):
deps = CMakeDeps(self)
deps.set_property("util-linux-libuuid", "cmake_find_mode", "both")
deps.set_property("util-linux-libuuid", "cmake_file_name", "libuuid")
deps.set_property("util-linux-libuuid", "cmake_target_name", "libuuid::libuuid")
deps.generate()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't disagree. I put this up to push the issue forward but I'm not particularly invested in an individual solution. I'm not sure your suggestion for recipename::recipename
is a good one here - we're talking about the target for an interface for which there is no standard. Ideally these three libuuid recipes would be completely substitutable (independently of how advisable that would be). recipename::recipename
doesn't satisfy that. If anything along those lines, I think it should be provides::provides
. Whether we change what's provided is another question I guess.
At the moment the recipe has provides = "libuuid"
, so I think libuuid::libuuid
makes the most sense. I think there's probably also an argument for provides = "uuid"
and uuid::uuid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity, the three libraries that I see implementing this interface are:
- https://git.kernel.org/pub/scm/fs/ext2/e2fsprogs.git/tree/lib/uuid (not in conan)
- https://github.com/util-linux/util-linux (this library)
- https://sourceforge.net/projects/libuuid/ (libuuid in conan)
See #17427 (comment) for discussion of each. The original implementation is uuid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example if in downstream library there is a custom
Findlibuuid.cmake
and its CMakeLists.txt usesfind_package(libuuid)
andlibuuid::libuuid
target:def generate(self): deps = CMakeDeps(self) deps.set_property("util-linux-libuuid", "cmake_find_mode", "both") deps.set_property("util-linux-libuuid", "cmake_file_name", "libuuid") deps.set_property("util-linux-libuuid", "cmake_target_name", "libuuid::libuuid") deps.generate()
I like this way of patching/modifying the conan defaults to what individual recipes require - i think it would be good to preference this over patching the build system files where possible. Having said that, given that we're talking about an implementation interface, as above, I think that the provides
interface should be used instead of recipename
, i.e.
- deps.set_property("util-linux-libuuid", "cmake_find_mode", "both")
+ deps.set_property("uuid", "cmake_find_mode", "both") # or libuuid/LibUUID, whatever value `provides` has
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm coming around to using uuid
as the provides
value. It's the name of the lib that's built by both recipes, which i guess is what we're trying to capture in an interface name:
self.cpp_info.libs = ["uuid"] |
self.cpp_info.libs = ["uuid"] |
so perhaps the target name should be uuid::uuid
For traceability, there is also relevant conversation about this in https://cpplang.slack.com/archives/C41CWV9HA/p1689414965702109 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully we can merge this in quick so we can adjust our names internally and stabilize on this. thx!
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@samuel-emrys @SpaceIm could we make progress on this so as to finalize the target names so that every depending recipe can unify and stabilize around the final name. thx! |
Hi @samuel-emrys - thank you for your contribution, and for the detailed analysis in #17427 (comment). You may have been this already, but indeed it does appear to be the case that the libuuid implementation on most Linux distros is the one from this recipe ( The conflicts with a system installed version are unfortunate, as it means that there are dependencies in potentially two places due to reliance on xorg/system (not that we can do much about it) - which makes me wonder whether a As for the CMake target name, this is rather unfortunate and caused by a defect in the I can see that because of the Conan-Center-only I think I would be inclined to keep As we update recipes to depend on |
This comment has been minimized.
This comment has been minimized.
@jcar87 Is it possible for us to have both a system and non-system recipe? I have some code that uses libuuid directly and I'd prefer to have it tied to a specific version of this library as opposed to the system one if possible, even if it would potentially conflict with the libuuid used by the xorg system package. |
Yep, that makes sense. Before we move on, could you just confirm the following for me?
This feels a bit like giving up as a package manager - I think the solution that @prince-chrismc proposed here is a better one - introduce a dependency in the
Can you confirm that you're aware that
So, just to make sure I understand, all of the class LibUUID(ConanFile):
provides = "uuid" and a cmake target of |
To an extent, this could be valid as well. However this would be an artificial dependency -
I'm unsure what you are referring to as "public" target. If it is a target defined in a When users consume any of the 10 recipes that depend on I think there is a strong case here that this is the (B) situation and that the |
I'm not sure this is the most robust solution. I see other problems with it. What if both a source version of The most robust solution imo would be to package
I see where there's been miscommunication. The discussion above was related to this statement:
This Find module is unofficial - not a module for public consumption. See @SpaceIm's review and the kitware discussion I think the options you have are good:
But I think (C) can be abstracted to
What i'm proposing is that both of these should use The way this applies to To be clear, all would share the same CMake code:
Does that make sense? Again, no problems if you don't like this approach - just want to make sure I'm getting my point across before moving on. |
i see what you mean re: Happy to proceed with this PR, but out of abundance of caution I would also keep I agree that it should be sensible for "provides" to share the same target name for transparent drop-in replacement. I'm not sure libjpeg and jpeg-turbo are the most perfect example of this, since jpeg-turbo is not a drop-in replacement in all cases (v7 and v8 emulation is opt-in, jpeg-turbo is not compatible with libjpeg v9 features), and jpeg-turbo also has its own API that is not compatible with libjpeg (should one choose to use that).
A system libuuid would also need a provides, and would also need to be incompatible with the other versions. A consumer can link against the system uuid coming from a libuuid conan packages that acts as proxy for the system one. The incompatibility would be there on purpose to avoid having different ones.
I was not involved when the |
fair comment - I'm not particularly familiar with either. Glad to see this is moving forward. |
Conan v1 pipeline ✔️All green in build 3 (
Conan v2 pipeline ✔️
All green in build 3 (
|
…D::LibUUID to libuuid::libuuid * [util-linux-libuuid] Change CMake target from LibUUID::LibUUID to libuuid::libuuid * This is in line with what conan recipes seem to expect, which will minimise the number of patches required. LibUUID was chosen as the target name given this is what's used internally within cmake, but this isn't a public interface and so there is no common usage to take guidance from. Closes conan-io#18554 * add alias for LibUUID::LibUUID --------- Co-authored-by: Luis Caro Campos <3535649+jcar87@users.noreply.github.com>
This is in line with what conan recipes seem to expect, which will minimise the number of patches required. LibUUID was chosen as the target name given this is what's used internally within cmake, but this isn't a public interface and so there is no common usage to take guidance from.
See discussion in original ticket adding
util-linux-libuuid
after merge: #17664 (comment)The following tickets are blocked until this change is merged:
Closes #18554